Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf_hooks: add resourcetiming buffer limit #44220

Closed

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Aug 12, 2022

Add WebPerf API performance.setResourceTimingBufferSize and event
'resourcetimingbufferfull' support.

The resource timing entries are added to the global performance timeline buffer automatically when using fetch. If users are not proactively cleaning these events, it can grow without limit. Apply the https://www.w3.org/TR/timing-entrytypes-registry/ default resource timing buffer max size so that the buffer can be limited to not grow indefinitely.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 12, 2022
@legendecas legendecas force-pushed the resource-timing-buffer-limit branch 2 times, most recently from 601bfad to 2579888 Compare August 12, 2022 06:41
@legendecas
Copy link
Member Author

@nodejs/diagnostics would you mind taking a look at this PR? thank you!

*/
function bufferResourceTiming(entry) {
if (resourceTimingBuffer.length >= resourceTimingBufferSizeLimit) {
dispatchBufferFull('resourcetimingbufferfull');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not too familiar with the spec, but is this timing expected? Seems to me like it makes more sense for the event to dispatch immediately after reaching capacity so the buffer can be consumed before the next event is received, not at the point the next event is received and discarding the event without having any chance to react before it is discarded. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, seems like this is a mismatch with the spec. I'll check again on the steps with the spec conformance. Thanks for pointing this out!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@legendecas legendecas marked this pull request as draft August 14, 2022 16:12
Add WebPerf API `performance.setResourceTimingBufferSize` and event
`'resourcetimingbufferfull'` support.

The resource timing entries are added to the global performance
timeline buffer automatically when using fetch. If users are not
proactively cleaning these events, it can grow without limit. Apply
the https://www.w3.org/TR/timing-entrytypes-registry/ default
resource timing buffer max size so that the buffer can be limited
to not grow indefinitely.
@legendecas legendecas marked this pull request as ready for review August 15, 2022 08:32
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 15, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 15, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 15, 2022

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (just a documentation suggestion).

doc/api/perf_hooks.md Show resolved Hide resolved
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 18, 2022
@legendecas
Copy link
Member Author

@Qard would you mind taking a look at this again? thank you.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 18, 2022
@nodejs-github-bot
Copy link
Collaborator

legendecas added a commit that referenced this pull request Aug 21, 2022
Add WebPerf API `performance.setResourceTimingBufferSize` and event
`'resourcetimingbufferfull'` support.

The resource timing entries are added to the global performance
timeline buffer automatically when using fetch. If users are not
proactively cleaning these events, it can grow without limit. Apply
the https://www.w3.org/TR/timing-entrytypes-registry/ default
resource timing buffer max size so that the buffer can be limited
to not grow indefinitely.

PR-URL: #44220
Reviewed-By: Rafael Gonzaga <[email protected]>
@legendecas
Copy link
Member Author

Landed in 798a6ed

@legendecas legendecas closed this Aug 21, 2022
@legendecas legendecas deleted the resource-timing-buffer-limit branch August 21, 2022 02:44
ruyadorno pushed a commit that referenced this pull request Aug 23, 2022
Add WebPerf API `performance.setResourceTimingBufferSize` and event
`'resourcetimingbufferfull'` support.

The resource timing entries are added to the global performance
timeline buffer automatically when using fetch. If users are not
proactively cleaning these events, it can grow without limit. Apply
the https://www.w3.org/TR/timing-entrytypes-registry/ default
resource timing buffer max size so that the buffer can be limited
to not grow indefinitely.

PR-URL: #44220
Reviewed-By: Rafael Gonzaga <[email protected]>
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
Add WebPerf API `performance.setResourceTimingBufferSize` and event
`'resourcetimingbufferfull'` support.

The resource timing entries are added to the global performance
timeline buffer automatically when using fetch. If users are not
proactively cleaning these events, it can grow without limit. Apply
the https://www.w3.org/TR/timing-entrytypes-registry/ default
resource timing buffer max size so that the buffer can be limited
to not grow indefinitely.

PR-URL: nodejs#44220
Reviewed-By: Rafael Gonzaga <[email protected]>
@juanarbol
Copy link
Member

This depends on #40532; marked as "dont-land-on-v16.x"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants